-
Notifications
You must be signed in to change notification settings - Fork 182
[CIR][HIP] Proper Handling of address spaces in ptr-diff #1994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| rhs = Builder.createAddrSpaceCast(rhs, lhsPtrTy); | ||
| } else if (lhsPtrTy != rhsPtrTy) { | ||
| // Same addrspace but different pointee/type → bitcast is fine | ||
| rhs = Builder.createBitcast(rhs, lhsPtrTy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Can you please try createPtrBitcast
I have a local patch for that function to allow passing AS, I'll upload it ASAP:
----------- clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h -----------
index b7b599e1289e..fd096ff97eb7 100644
@@ -574,7 +574,7 @@ public:
mlir::Value createPtrBitcast(mlir::Value src, mlir::Type newPointeeTy) {
assert(mlir::isa<cir::PointerType>(src.getType()) && "expected ptr src");
- return createBitcast(src, getPointerTo(newPointeeTy));
+ return createBitcast(src, getPointerTo(newPointeeTy, mlir::cast<cir::PointerType>(src.getType()).getAddrSpace()));
}In addition I have another patch to fix the case of different AS and different data type. I'll upload them ASAP and let you know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to use createPtrBitcast. When you patch lands and fixes AS I will be happy to revise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I took a closer look at createPtrBitcast, and in this particular spot I already have the full destination pointer type (lhsPtrTy) available, including the address space information. Using createBitcast(rhs, lhsPtrTy) lets me preserve that exact type without reconstructing it.
createPtrBitcast is super useful in places where we only know the new pointee type, but here we already computed the exact pointer type we want to cast to, so sticking to createBitcast keeps the intent clearer.
Happy to switch if we move toward a uniform helper style later, but for this case preserving the full cir::PointerType seems cleaner.
Thanks again for taking a look — really appreciate the review!
|
|
||
| mlir::Type lhsTy = lhs.getType(); | ||
| mlir::Type rhsTy = rhs.getType(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should follow OG skeleton here, similar names if possible would be helpful:
// Okay, figure out the element size.
const BinaryOperator *expr = cast<BinaryOperator>(op.E);
QualType elementType = expr->getLHS()->getType()->getPointeeType();
...
// For a variable-length array, this is going to be non-constant.
if (const VariableArrayType *vla
= CGF.getContext().getAsVariableArrayType(elementType)) {
llvm_unrecheable("NYI");
...
else {
...
}
No description provided.